Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Ensure submodule inclusion + Fix the tests #104

Merged
merged 9 commits into from
Jan 14, 2021
Merged

Ensure submodule inclusion + Fix the tests #104

merged 9 commits into from
Jan 14, 2021

Conversation

@aminya
Copy link
Contributor

@aminya aminya commented Dec 30, 2020 •
edited
Loading

Description of the change

  • This ensures that the submodules are downloaded before publishing to npm
  • Update temp to fix the tests on Windows

Since #103 is not still merged:

  • Updated the Travis and Appveyor configs just to get CI working for now: Use Node 12 and MSVC 2015 in the CI
  • Regenerated package-lock.json

Benefits

Fixes the Atom bootstrap error:

Details 
make: *** No rule to make target 'Release/obj.target/libgit2/deps/libgit2/src/annotated_commit.o', needed by 'Release/obj.target/git2.a'.  Stop.
gyp ERR! build error 
gyp ERR! stack Error: `make` failed with exit code: 2
gyp ERR! stack     at ChildProcess.onExit (/usr/lib/node_modules/node-gyp/lib/build.js:194:23)
gyp ERR! stack     at ChildProcess.emit (node:events:376:20)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (node:internal/child_process:277:12)
gyp ERR! System Linux 5.9.11-3-MANJARO
gyp ERR! command "/usr/bin/node" "/usr/lib/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
gyp ERR! cwd /home/illright/atom/apm/node_modules/atom-package-manager/node_modules/git-utils
gyp ERR! node -v v15.3.0
gyp ERR! node-gyp -v v7.1.0
gyp ERR! not ok

Fixes #69
Closes #105

Verification

The CI and the tests pass:
image

After publishing

Atom and apm should bump their dependency to 5.7.1.

@aminya
Copy link
Contributor Author

aminya commented Dec 30, 2020

@sadick254 @darangi This bug prevents people to be able to bootstrap Atom. Could you merge it as soon as possible? It should be released as 5.7.1

DeeDeeG
DeeDeeG reviewed Dec 30, 2020
View reviewed changes
DeeDeeG
DeeDeeG reviewed Dec 31, 2020
View reviewed changes
@DeeDeeG

This comment has been minimized.

Sign in to view
@aminya
Copy link
Contributor Author

aminya commented Dec 31, 2020 •
edited
Loading

It is fixed in this commit. I would recommend closing the PR to speed up the process.

@aminya aminya changed the title fix: use prepare to init the submodule Ensure submodule inclusion + Fix the tests on Windows Dec 31, 2020
@aminya aminya changed the title Ensure submodule inclusion + Fix the tests on Windows Ensure submodule inclusion + Fix the tests Dec 31, 2020
DeeDeeG
DeeDeeG reviewed Dec 31, 2020
View reviewed changes
@DeeDeeG DeeDeeG mentioned this pull request Dec 31, 2020
Closed
This was referenced Dec 31, 2020
Closed
Draft
DeeDeeG
DeeDeeG reviewed Jan 12, 2021
View reviewed changes
@aminya aminya force-pushed the submodule-init branch 2 times, most recently from fbd28b1 to 4f2bdc8 Compare January 12, 2021 20:19
@aminya
Copy link
Contributor Author

aminya commented Jan 12, 2021 •
edited
Loading

@sadick254 The issue that this PR fixes is much important than what I see has been merged yesterday. Could you merge this?

Atom cannot be bootstrapped because of this.
Apm cannot be bootstrapped because of this.

FYI, 1 person has left our atom-community organization since two weeks ago. 😞

@DeeDeeG DeeDeeG mentioned this pull request Jan 12, 2021
Closed
1 task
@DeeDeeG
Copy link
Contributor

DeeDeeG commented Jan 12, 2021

@sadick254 I posted an issue #109 to fully explain the issue, but basically git-utils@v5.7.0 is broken and we would request that it be re-uploaded, perhaps as v5.7.1. Because the deps/libgit2 submodule is missing from that version.

This PR (or a similar approach) would make it so the submodule initializes automatically during the npm publish process. I am maybe not 100% sure of every detail of this PR, but the community fork of Atom is currently affected. So some sort of resolution would be much appreciated. The community fork of Atom is trying to pull in this version of git-utils and that is preventing the fork from building. The issue may also interfere with upstream/official Atom some time in the future if not addressed. Sorry to bother you about it, but @aminya is right, it is a somewhat pressing issue IMO.

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Jan 13, 2021 •
edited
Loading

@aminya if we merge atom-community/apm#70 and update our copy of apm then we can work around this by installing apm with exact dependencies (with npm ci).

Eventually having a new version of git-utils published with the submodule included would be much appreciated.

sadick254
sadick254 approved these changes Jan 14, 2021
View reviewed changes
Copy link
Contributor

@sadick254 sadick254 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great.

@sadick254 sadick254 merged commit a265d04 into atom:master Jan 14, 2021
@DeeDeeG
Copy link
Contributor

DeeDeeG commented Jan 14, 2021

Thank you for merging, and for publishing the new release!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@DeeDeeG DeeDeeG DeeDeeG left review comments

@sadick254 sadick254 sadick254 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

node-gyp rebuild fails

3 participants

@aminya @DeeDeeG @sadick254